Fix Subnet update error detection #991
Conversation
yanjunz97
force-pushed
the
fix-subnet-update
branch
from
3f70328 to
1942e20
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
==========================================
+ Coverage 73.43% 74.17% +0.73%
==========================================
Files 118 118
Lines 16362 16358 -4
==========================================
+ Hits 12015 12133 +118
+ Misses 3564 3454 -110
+ Partials 783 771 -12
|
pkg/nsx/services/subnet/subnet.go
Outdated
| } | ||
| } | ||
| if !changed { | ||
| log.Info("Subnet not changed, skip updating", "SubnetId", uid) | ||
| return existingSubnet, nil | ||
| } | ||
| } | ||
| return service.createOrUpdateSubnet(obj, nsxSubnet, &vpcInfo) | ||
| return service.createOrUpdateSubnet(obj, nsxSubnet, &vpcInfo, isUpdate) |
There was a problem hiding this comment.
it is a little confusing to have both changed and isUpdate, maybe just use changed & !existingSubnet to pass to the parameter in createOrUpdateSubnet
| vpcSubnets[i].Tags = newTags | ||
| // Avoid updating vpcSubnets[i] to ensure Subnet store | ||
| // is only updated after the updating succeeds. | ||
| updatedSubnet := *vpcSubnets[i] |
There was a problem hiding this comment.
Why we need two new variables comparableSubnet and updatedSubnet here
There was a problem hiding this comment.
It makes sense to merge them, use only updatedSubnet instead, thanks!
| @@ -398,7 +398,7 @@ func TestSubnetService_UpdateSubnetSet(t *testing.T) { | |||
| }) | |||
|
|
|||
| patchesCreateOrUpdateSubnet := gomonkey.ApplyFunc((*SubnetService).createOrUpdateSubnet, | |||
| func(r *SubnetService, obj client.Object, nsxSubnet *model.VpcSubnet, vpcInfo *common.VPCResourceInfo) (*model.VpcSubnet, error) { | |||
| func(r *SubnetService, obj client.Object, nsxSubnet *model.VpcSubnet, vpcInfo *common.VPCResourceInfo, isUpdate bool) (*model.VpcSubnet, error) { | |||
There was a problem hiding this comment.
Do you think we need a new UT for createOrUpdateSubnet?
There was a problem hiding this comment.
Yes, some part of the code in createOrUpdateSubnet is not covered yet. Added the unit test, thanks
pkg/nsx/services/subnet/subnet.go
Outdated
| // For update case, wait for some time to make sure the realized state is updated. | ||
| if isUpdate { | ||
| time.Sleep(1 * time.Second) | ||
| } |
There was a problem hiding this comment.
it's better that NSX fix that issue. After NSX patch API called, the realized status should be updated in sync mode instead
There was a problem hiding this comment.
Opened a bug for NSX team, let's see how they reply on this observations, thanks
yanjunz97
force-pushed
the
fix-subnet-update
branch
2 times, most recently
from
06fa16e to
534800e
Compare
Signed-off-by: Yanjun Zhou <[email protected]>
yanjunz97
force-pushed
the
fix-subnet-update
branch
from
534800e to
1da7143
Compare
Signed-off-by: Yanjun Zhou <[email protected]>
There are 2 issues in Subnet update error detection:
while NSX may haven't updated the RealizedState for the updated Subnet.
This PR add a delay before the RealizedState check for the Subnet update case.
which results in the next reconcile will find the updated Subnet already in store,
skip the update, and overwrite the Status as Ready.
This PR uses a copy when updating the Subnet to avoid this issue.